Skip to content

[Common] Fix Build: NCCL EP build to respect MAX_JOBS - #3138

Merged
phu0ngng merged 2 commits into
NVIDIA:mainfrom
phu0ngng:te_ep/fix_build
Jul 22, 2026
Merged

[Common] Fix Build: NCCL EP build to respect MAX_JOBS#3138
phu0ngng merged 2 commits into
NVIDIA:mainfrom
phu0ngng:te_ep/fix_build

Conversation

@phu0ngng

@phu0ngng phu0ngng commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Description

NCCL EP build to respect MAX_JOBS.

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Infra/Build change
  • Code refactoring

Checklist:

  • I have read and followed the contributing guidelines
  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@phu0ngng phu0ngng changed the title [Common]Fix Build: Remove nproc from parallel make for NCCL EP build [Common] Fix Build: Remove nproc from parallel make for NCCL EP build Jun 22, 2026
@phu0ngng

Copy link
Copy Markdown
Collaborator Author

/te-ci L0

@greptile-apps

greptile-apps Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates the NCCL EP build in setup.py to respect the MAX_JOBS environment variable (and NVTE_BUILD_MAX_JOBS) by replacing the hardcoded os.cpu_count() or 8 with the shared get_max_jobs_for_parallel_build() utility already used by the CMake build path.

  • get_max_jobs_for_parallel_build() returns 0 as a sentinel for "no limit", but the make invocation in setup.py unconditionally passes str(nproc) to -j, producing make -j 0 when no job limit is configured. The parallel build_ext.py call site correctly guards this with if max_jobs > 0; the same pattern is needed here.
  • When a valid job count is provided via MAX_JOBS, the change works as intended and aligns the NCCL EP build with the rest of the TE build system.

Confidence Score: 4/5

Safe to merge if the make -j 0 edge case is addressed; builds that explicitly set MAX_JOBS work correctly today.

When no job-limit environment variable is set, get_max_jobs_for_parallel_build() returns 0 and the NCCL EP make invocation becomes make -j 0, whose behavior is undefined across GNU make versions. Builds with an explicit MAX_JOBS are unaffected, but the default (no-limit) path is broken in a way that depends on the host's make version.

setup.py — the make invocation needs the same if nproc > 0 guard that build_ext.py already uses for its CMake parallel argument.

Important Files Changed

Filename Overview
setup.py Replaces hardcoded os.cpu_count() or 8 with get_max_jobs_for_parallel_build(), but does not guard against the 0 sentinel (unlimited) returned by that function, resulting in make -j 0 when no job limit is configured.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build_nccl_ep_submodule] --> B[get_max_jobs_for_parallel_build]
    B --> C{env var set?}
    C -->|NVTE_BUILD_MAX_JOBS or MAX_JOBS| D[return N gt 0]
    C -->|none set| E[return 0]
    D --> F["make -j N (correct)"]
    E --> G["make -j 0 (undefined behavior)"]
    style G fill:#f66,color:#fff
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into te_ep/fix_build" | Re-trigger Greptile

Comment thread setup.py
@phu0ngng
phu0ngng marked this pull request as draft June 30, 2026 13:22
@phu0ngng phu0ngng changed the title [Common] Fix Build: Remove nproc from parallel make for NCCL EP build [Common] Fix Build: NCCL EP build to respect MAX_JOBS Jun 30, 2026

@timmoon10 timmoon10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix at #3034 (comment) will reuse our existing infrastructure.

Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@phu0ngng
phu0ngng requested a review from timmoon10 July 8, 2026 20:07
@phu0ngng
phu0ngng marked this pull request as ready for review July 8, 2026 20:07
@phu0ngng

phu0ngng commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/te-ci L0

@timmoon10 timmoon10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@phu0ngng
phu0ngng merged commit 4adad4c into NVIDIA:main Jul 22, 2026
9 of 14 checks passed
@phu0ngng
phu0ngng deleted the te_ep/fix_build branch July 22, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants